xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java (1)

61-73: 💤 Low value

SPI factory name collisions will throw at startup.

If an SPI-discovered SotwConfigSourceSubscriptionFactory has the same name() as the built-in PathSotwConfigSourceSubscriptionFactory or GrpcConfigSourceStreamFactory, the ImmutableMap.Builder will throw IllegalArgumentException on duplicate keys. This may be intentional to prevent accidental overrides, but consider documenting this behavior or adding a more descriptive error message.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java` around
lines 61 - 73, SPI-discovered SotwConfigSourceSubscriptionFactory instances can
have the same name() as built-in factories
(PathSotwConfigSourceSubscriptionFactory or GrpcConfigSourceStreamFactory),
causing ImmutableMap.Builder to throw on duplicate keys; add an explicit
duplicate-name check before calling register (or inside register) that detects
if byName already contains the factory.name() and throw a clear
IllegalStateException (or log+skip) that includes the duplicate name and both
factory class names (e.g., reference PathSotwConfigSourceSubscriptionFactory,
GrpcConfigSourceStreamFactory, SotwConfigSourceSubscriptionFactory, register,
byName, byTypeUrl) so the startup failure message is descriptive.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PathConfigSourceTest.java (1)

127-152: ⚖️ Poor tradeoff

Consider adding a test for dynamic file-update delivery.

The helper verifyClusterFromPathConfigSource only verifies the initial load path. The primary value of path_config_source is reacting to file changes at runtime. A complementary test that overwrites the file after XdsBootstrap is running and asserts the new snapshot is delivered would give meaningful coverage of the watch loop in PathConfigSourceSubscription.parseAndPush.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PathConfigSourceTest.java`
around lines 127 - 152, The test helper verifyClusterFromPathConfigSource only
asserts initial load; add a new test that starts XdsBootstrap with
verifyClusterFromPathConfigSource (or reuse its setup) and then updates the file
backing the PathConfigSource to a different cluster name, then await until the
SnapshotWatcher (set as defaultSnapshotWatcher) observes a new ClusterSnapshot
with the new resource name. Specifically, reuse the
AtomicReference<ClusterSnapshot> and SnapshotWatcher logic from
verifyClusterFromPathConfigSource, perform a file overwrite of the path-config
file after xdsBootstrap.clusterRoot("path-cluster") is active, and assert the
watcher receives the updated snapshot (verifying
PathConfigSourceSubscription.parseAndPush reacts to the file change and pushes
the new snapshot).

it/xds-controlplane-api/src/test/java/com/linecorp/armeria/xds/it/ControlPlaneApiRegressionTest.java (1)

115-119: ⚡ Quick win

Fail fast on watcher callback errors instead of silently ignoring them.

At Line 115 and Line 186, the callback drops t. If the watcher reports an error, these tests may only fail by timeout. Capture t and assert it is null inside await() for clearer failures.

Suggested change

-        final AtomicReference<ClusterSnapshot> snapshotRef = new AtomicReference<>();
+        final AtomicReference<ClusterSnapshot> snapshotRef = new AtomicReference<>();
+        final AtomicReference<Throwable> errorRef = new AtomicReference<>();
         try (XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap)) {
             xdsBootstrap.clusterRoot("my-cluster")
                         .addSnapshotWatcher((snapshot, t) -> {
+                            if (t != null) {
+                                errorRef.set(t);
+                                return;
+                            }
                             if (snapshot != null) {
                                 snapshotRef.set(snapshot);
                             }
                         });

             await().untilAsserted(() -> {
+                assertThat(errorRef.get()).isNull();
                 assertThat(snapshotRef.get()).isNotNull();
                 assertThat(snapshotRef.get().xdsResource().resource().getName())
                         .isEqualTo("my-cluster");
             });
         }

Also applies to: 186-190

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@it/xds-controlplane-api/src/test/java/com/linecorp/armeria/xds/it/ControlPlaneApiRegressionTest.java`
around lines 115 - 119, The watcher callback passed to addSnapshotWatcher
currently ignores the throwable parameter t, causing test failures to surface
only via timeouts; modify the callback used in ControlPlaneApiRegressionTest
(the lambda passed to addSnapshotWatcher that updates snapshotRef) to capture
the throwable (t) into a shared AtomicReference or similar (e.g., errorRef) and
then, in the await() assertion after the watcher runs, assert that that errorRef
is null (or rethrow/assert the throwable) so any watcher error fails fast and
surfaces immediately; apply the same change to the second occurrence around
lines referenced (the other addSnapshotWatcher callback).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

xds/src/main/java/com/linecorp/armeria/xds/PathSotwConfigSourceSubscriptionFactory.java (2)

101-121: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Watcher-registration failure handling still incomplete

The catch block silently calls lifecycleObserver.close() from the blocking executor thread (not the event loop), which is inconsistent with every other lifecycle-observer call in this class (all of which happen on the event loop via parseAndPush or close0). Additionally, the failure is still not surfaced to callbacks, leaving the subscription silently inoperative.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@xds/src/main/java/com/linecorp/armeria/xds/PathSotwConfigSourceSubscriptionFactory.java`
around lines 101 - 121, In startWatching(), the catch block currently closes
lifecycleObserver from the blocking executor and doesn't notify the subscription
callbacks; instead, schedule the failure handling onto the eventLoop: inside the
catch, capture the exception and call eventLoop.execute(() -> {
lifecycleObserver.close(); close0(null) or the appropriate cleanup; invoke the
subscription callbacks with the error (e.g., callbacks.onError /
callbacks.onSubscriptionFailed or the class's existing error-reporting callback)
so the failure is surfaced; ensure watchCancellable remains null. Reference:
startWatching, watchService.register, lifecycleObserver, eventLoop, close0,
watchCancellable, callbacks, parseAndPush.

---

61-67: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validation still missing in create()

configSource.getPathConfigSource() is called without first checking configSource.hasPathConfigSource(), and neither configSource nor callbacks nor eventLoop are null-checked. This is unchanged since the prior review.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@xds/src/main/java/com/linecorp/armeria/xds/PathSotwConfigSourceSubscriptionFactory.java`
around lines 61 - 67, In create(), validate inputs before using them: check
configSource is non-null and configSource.hasPathConfigSource() is true before
calling configSource.getPathConfigSource(), and null-check callbacks and
eventLoop; if any validation fails, throw an appropriate exception (e.g.,
IllegalArgumentException or NullPointerException) with a clear message. After
these guards, construct and return the PathConfigSourceSubscription using the
verified configSource.getPathConfigSource(), watchService, callbacks, eventLoop,
meterRegistry, and meterIdPrefix.